fix: fix false positives in encodingExists#328
Conversation
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
|
cc: @jonchurch @UlisesGascon @Phillip9587 btw this issue also affects body-parser. Without this change, the error thrown by body-parser is at https://github.com/expressjs/body-parser/blob/0dad12f03792ee4991d1054b1f5ff35ed70a3e34/lib/read.js#L124, whereas with this change it’s at https://github.com/expressjs/body-parser/blob/0dad12f03792ee4991d1054b1f5ff35ed70a3e34/lib/read.js#L66. It will always throw an error, but now in different place. |
| iconv.encodings = require("../encodings"); // Lazy load all encoding definitions. | ||
| if (!iconv.encodings) { | ||
| var raw = require("../encodings"); | ||
| iconv.encodings = objectAssign(Object.create(null), raw); // Lazy load all encoding definitions. |
There was a problem hiding this comment.
one minor nit - I've tried my best to keep the set of dependencies minimal, not sure how important it is in today's standards. It seems that here it's a matter of a simple for loop to assign the properties. I'll leave it to you to make a decision of whether it's worth it.
There was a problem hiding this comment.
Yep, I was looking at the past commits and issues. In this case, the bundler barely increases. In Webpack or other bundlers, depending on the ECMAScript version they are compiling, it might reduce more. However, in this case, we’re using Webpack in development mode, for production, it should reduce even more.
master branch:
This pr
2 participants
Since the object that holds the encodings has a prototype, it can cause false positives when searching for it internally. This also applies to the internal cache that this library has
The solution is to create an object with no prototype, with this change, when using encodingExists it will no longer return true for properties from the prototype.
Either way, iconv.decode or iconv.encode, when trying to access an encoding called 'constructor', would throw an error because they don’t have the necessary methods to perform the encoding transformation.
closes #323